fix(presets): recompose surviving skills when a lower preset layer is removed by yxshee · Pull Request #5013 · github/spec-kit

yxshee · 2026-10-17T19:15:02Z

Description

Removing a preset that is the base, or a middle layer, of another preset's prepend/append/wrap composition leaves the removed preset's content in the active agent's SKILL.md. That affects Claude, Codex, Copilot with --skills, and other skills-based integrations. specify preset resolve reports the new chain, and command-file agents such as Gemini are recomposed onto it. But the skill keeps the removed preset's instructions, so /speckit-specify runs content the user just removed.

Reproduction (alpha replaces speckit.specify with ALPHA BODY; beta appends BETA APPENDED):

$ specify init p --integration claude --ignore-agent-tools --non-interactive && cd p
$ specify preset add --dev ../alpha --priority 5
$ specify preset add --dev ../beta --priority 2
$ specify preset remove alpha
✓ Preset 'alpha' removed successfully
$ grep -c "ALPHA BODY" .claude/skills/speckit-specify/SKILL.md
1          # main; 0 with this change
$ specify preset resolve speckit.specify
    (top layer from: beta v1.0.0)
    1. [base] core (bundled) → ...
    2. [append] beta v1.0.0 → ...

The same steps with --integration gemini drop ALPHA BODY from .gemini/commands/speckit.specify.toml on main, so only skills output is stale.

Root cause, in presets/_manager_skills.py:

On removal, _reconcile_skills() only revisits the active skills directory when _unregister_skills() restored something there (if extra_skills_dirs is None or active_provenance).
The composed skill is owned by the surviving composing preset (beta), not the removed one, so nothing is restored and the active directory is skipped.
The recomposed .composed/<cmd>.md that _reconcile_composed_commands() writes is therefore never rendered into the skill.
The condition came in with fix: register extensions for the active integration only #3459 (683bfd0) and moved in the refactor(presets): split domain internals into private modules #4747 refactor. Before fix: register extensions for the active integration only #3459 the active directory was always reconciled.

The fix:

_reconcile_skills() now records commands whose winning layer composes (strategy other than replace).
After a removal, it also re-renders those skills in the active directory, but only when the SKILL.md still carries the winning preset's metadata.source: preset:<id> marker. A new helper, _skills_owned_by_preset(), does that check.
A hand-edited skill is left alone, matching the existing protection for non-owned skills during removal.
The install and rescaffold paths are unchanged.

Testing

Tested locally with uv run specify --help
Ran existing tests with uv sync && uv run pytest
Tested with a sample project (if applicable): the reproduction above with claude (on main and this branch) and gemini (on main)

New tests in tests/specify_cli/presets/test_manager_skills.py:

test_remove_base_preset_recomposes_surviving_composed_skill: fails on main with assert "Base preset body" not in content
test_remove_middle_preset_drops_its_layer_from_composed_skill: fails on main with assert "Middle body" not in content
test_remove_base_preset_preserves_hand_edited_composed_skill: the negative case, guarding the hand-edit protection; passes before and after

With the change the file gives 70 passed.

Preset, integration, bundle and extension suites: 6353 passed, 9 skipped.
Full suite (macOS, Python 3.13): 12332 passed, 376 skipped. 11 failures in the tests/test_*_python_parity.py files occur identically on unmodified main on this machine under full-suite load; those files pass when run on their own (257 passed, 112 skipped).
One more local failure, test_resolver.py::TestResolveCore::test_resolve_core_does_not_return_preset_files, was an artifact of a worktree directory literally named presets: the test asserts "presets" not in path.parts. It fails identically on main in that directory and passes elsewhere.
uvx ruff@0.15.0 check src tests: clean

AI Disclosure

I did not use AI assistance for this contribution
I did use AI assistance (fill in the disclosure below)

AI disclosure: Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode) opened this PR on behalf of @yxshee. A Claude Code subagent (same model, autonomous) found the bug and wrote the fix and tests. The lead agent independently re-ran the new tests against main and the branch, reproduced the behavior with the real CLI (Claude and Gemini), confirmed the #3459 origin with git log -S, reviewed the diff, and wrote this description. No human reviewed the change line by line before it was opened.

🤖 Generated with Claude Code

… removed Removing a preset that is the base (or a middle layer) of another preset's prepend/append/wrap composition left the removed preset's content in the active agent's SKILL.md (Claude, Codex, Copilot with --skills, ...), while command-file agents such as Gemini were recomposed onto the new stack. The composed skill is owned by the surviving preset at the top of the chain, so _unregister_skills() restores nothing for the removed preset, and _reconcile_skills() only revisited the active skills directory when that restore had touched it. The recomposed .composed/<cmd>.md that _reconcile_composed_commands() writes was therefore never rendered into the skill, and `specify preset resolve` disagreed with what the agent ran. After a removal, also reconcile active-directory skills for commands whose winning layer composes, as long as the skill still carries the winning preset's source marker. A hand-edited skill is left alone, matching the existing protection for non-owned skills during removal. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Removal can overwrite a user-owned legacy skill despite the ownership filter.

1 open issue

Preset removal overwrites user-owned legacy skills · New

What changed in this PR

Updates Spec Kit preset removal so surviving composed skills reflect the remaining preset layers.

Changes:

Re-renders active composed skills that retain the winning preset’s ownership marker.
Adds regression coverage for base/middle removal and preservation of non-owned skills.

Validation used an isolated ownership check; project tests were not run.

AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this review for the requesting reviewer.

File	Description
tests/specify_cli/presets/test_manager_skills.py	Adds composition-removal and preservation tests.
src/specify_cli/presets/_manager_skills.py	Adds ownership-based reconciliation for surviving composed skills.

_{Review effort: Max}

When both the modern (speckit-specify) and legacy (speckit.specify) skill directories exist, the active-directory recomposition added for lower-layer removal selected the command because the modern skill kept the winning preset's marker, and _register_skills() then rewrote both directories, overwriting a user-owned legacy skill. Give _register_skills() an optional only_skill_names filter and pass the directory's managed names on removal, so only names that are still preset-owned are written. Install and rescaffold calls pass None and keep writing every existing directory, as before. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

yxshee · 2026-10-19T13:31:35Z

Review round 1, posted on behalf of @yxshee.

Addressed the Copilot finding that removal recomposition could overwrite a user-owned legacy skill (commit 8c0328f):

_register_skills() gains an optional only_skill_names filter; removal passes the directory's managed names for the active directory. Install and rescaffold behavior is unchanged.
New regression test_remove_base_preset_preserves_user_owned_legacy_skill: fails on the previous head (1239cbe), passes now.
tests/specify_cli/presets, tests/integrations, tests/specify_cli/integrations, tests/specify_cli/extensions, tests/specify_cli/bundles, tests/test_presets.py, tests/test_extension_skills.py: 6096 passed, 9 skipped. uvx ruff@0.15.0 check src tests: clean.

AI disclosure: written and pushed by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the code, test and this comment are AI-generated.

🤖 Addressed by Claude Code

Copilot

🟡 Changes recommended

Removing the sole composition base still re-renders cached instructions from the removed preset.

1 open issue

Invalidate stale composed cache when command has no fallback · New

1 resolved since last review

Preset removal overwrites user-owned legacy skills

_{Review effort: Max}

mnriem · 2026-10-19T14:16:13Z

Please address Copilot feedback

When the removed preset was the only replace base of a composing command, with no core or extension fallback, resolution returns None. Command-file agents unregister that command, but the survivor's stale .composed/<cmd>.md stayed behind, and the removal recomposition rendered it back into the skill, so the removed preset's instructions stayed active. Track composing winners whose stack no longer resolves, delete their stale composed cache, and on removal delete the active skill only while it still carries the winning preset's source marker. Hand-edited skills are kept, matching the existing ownership protection. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

yxshee · 2026-10-19T14:26:40Z

Review round 2, posted on behalf of @yxshee.

Addressed the Copilot finding about removing a command's only base with no core or extension fallback (commit 57bdfc9):

The stale .composed/<cmd>.md of a composing winner whose stack no longer resolves is deleted, so it can't be rendered back into a skill.
On removal, the active skill for that command is deleted only while it still carries the winner's source marker; hand-edited skills are kept.
New tests: test_remove_sole_base_preset_retires_stale_composed_skill (fails on the previous head, passes now) and test_remove_sole_base_preset_keeps_hand_edited_skill.
Preset, integration, extension and bundle suites: 6098 passed, 9 skipped. uvx ruff@0.15.0 check src tests: clean.

AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.

🤖 Addressed by Claude Code

Copilot

🟡 Changes recommended

Cache cleanup can delete external files, and removed preset instructions can remain runnable.

3 open issues

1 previously missed issue

In code that hasn't changed since last review

Handle aliases during preset retirement and recomposition

src/specify_cli/presets/_manager_skills.py:465

This loop checks only the primary command's modern and legacy names, not its manifest aliases. On Codex, an appending preset can declare speckit.alt as an alias of speckit.custom; removing its only base retires speckit-custom but leaves speckit-alt/SKILL.md runnable with the removed instructions. agents.py:934–1015 renders aliases separately, using <id>:<file> source markers, so the preset:<id> check also misses their ownership. Include the winning template's aliases and their actual ownership markers in recomposition and retirement, preserve user-owned files, and add a native-skill alias regression.

AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated this feedback for the requesting reviewer.

1 resolved since last review

Invalidate stale composed cache when command has no fallback

_{Review effort: Max}

Round-3 review of the no-base removal fix: - Delete a cache only when no path component below the project root is a symlink, so a symlinked .composed directory can't redirect the deletion to a file outside the project. - Drop every surviving preset's .composed/<cmd>.md for an uncomposable command, not only the winner's: a middle preset's stale cache could be rendered back into the skill by a later rescaffold. - Delete through the journal-aware unlink helper, as the cache writes already do, so a failing integration lifecycle can restore the cache. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mnriem · 2026-10-19T15:26:00Z

Please address Copilot feedback

yxshee · 2026-10-19T16:34:16Z

Review round 3, posted on behalf of @yxshee.

Addressed the three Copilot findings on stale composed-cache deletion (commit c184e1a):

Deletion skips any cache reached through a symlinked path component below the project root.
All surviving presets' caches for an uncomposable command are dropped, so a rescaffold can't render the removed base back.
Deletion uses the journal-aware unlink helper, like the cache writes.
Three new tests, each failing on the previous head. Preset, integration, extension and bundle suites: 6101 passed, 9 skipped. uvx ruff@0.15.0 check src tests: clean.

AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.

🤖 Addressed by Claude Code

Copilot

🔵 Needs a closer look

Destructive cleanup spans multiple lifecycle paths and still has unresolved safety and correctness issues.

1 open issue

Reject parent traversal in composed cache cleanup keys · New

3 previously missed issues

In code that hasn't changed since last review

Invalidate stale caches for disabled presets

src/specify_cli/presets/_manager_skills.py:208

presets_by_priority excludes disabled presets (_registry.py:227), so their stale caches survive this cleanup. With a replace base beneath two append presets, disable the middle preset and then remove the sole base: the middle cache still contains the removed instructions. Re-enabling it and rescaffolding can render that cache before reconciliation deletes it; rescaffold reconciliation does not retire the newly written skill. Invalidate caches for all installed presets, including disabled ones, and extend the three-layer regression to cover disabling and re-enabling the middle layer.

AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.

Preserve output when surviving layers cannot be read

src/specify_cli/presets/_manager_skills.py:211

resolve_content() also returns None when a surviving layer cannot be read or decoded (_resolver.py:780–783,865–871). Treating every None as a missing base deletes the last valid cache and owned skill even though a replace base still exists. A dependency-free execution with a non-UTF-8 base confirmed both deletions. Only retire output when the stack has no replace layer; preserve existing output on read failures, as test_unregister_skills_in_dir_unreadable_core_template_skips already requires for restoration. Add a composed-stack regression for this case.

AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.

Journal skill-directory removals during force replacement

src/specify_cli/presets/_manager_skills.py:434

This rmtree() bypasses the lifecycle file-change journal. Force replacement calls remove() from _manager.py:408; when that runs under an active journal, a later failure cannot restore this deleted skill, although cache deletion is observed. design/integration.md, “Storage, loading, and lifecycle,” requires preset replacements and generated artifacts to participate in the journal. Wrap this removal like the other skill deletions at lines 1377–1381 and 1644–1648, and extend the observer regression to check the skill-directory removal too.

AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.

3 resolved since last review

_{Review effort: Max}

…lures Round-4 review of the no-base removal fix: - Skip registry keys (and command names) that are not a single plain path segment, so a malformed key such as "../../../outside" cannot steer cache cleanup outside the project. - Invalidate stale caches for every installed preset, disabled ones included: re-enabling a disabled composer and rescaffolding could otherwise render its stale cache back. - Retire output only when the stack has no replace layer left. resolve_content() also returns None when a surviving layer can't be read; that keeps the existing cache and skill instead of deleting them. - Journal the stale skill-directory removal like the other skill deletions, so a failing lifecycle can restore it. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mnriem · 2026-10-19T17:23:50Z

Please address Copilot feedback

yxshee · 2026-10-19T17:31:07Z

Review round 4, posted on behalf of @yxshee.

Addressed the new finding and the three previously missed ones (commit b9a2c35):

Traversing registry keys: cleanup accepts only plain single-segment preset ids and command names.
Disabled presets: stale caches are invalidated for every installed preset, so re-enabling and rescaffolding can't render the removed base back.
Read failures: output is retired only when no replace layer remains; an unreadable surviving base keeps the existing cache and skill.
Journal: the stale skill-directory removal is wrapped in before_file_change/after_file_change like the other skill deletions.
Regressions: test_stale_cache_cleanup_ignores_traversing_registry_key, test_remove_sole_base_drops_disabled_preset_cache, test_unreadable_surviving_base_keeps_composed_output, and the journal test now also checks the skill directory; each fails on the previous head.
Preset, integration, extension and bundle suites: 6104 passed, 9 skipped. uvx ruff@0.15.0 check src tests: clean.

AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.

🤖 Addressed by Claude Code

Copilot

🔵 Needs a closer look

File-deleting reconciliation spans multiple lifecycle paths, with unresolved safety and ownership gaps requiring changes and human review.

1 open issue

Reject Windows junctions during cache cleanup · New

3 previously missed issues

In code that hasn't changed since last review

Limit historical-directory reconciliation to managed skill names

src/specify_cli/presets/_manager_skills.py:395

The new name filter protects only the active directory. The historical-directory branch below still calls _register_skills() without only_skill_names, so it writes every existing modern/legacy sibling. If removal restored only speckit-specify in an inactive agent's directory and preserved a user-owned speckit.specify beside it, reconciliation overwrites that preserved file with the surviving preset's content. Pass managed_names to the targeted call too, and add a regression with mixed ownership in a non-active agent directory.

AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.

Retire stale skills during activation and rescaffolding

src/specify_cli/presets/_manager_skills.py:453

This retirement only runs during removal, but activation and rescaffolding call _reconcile_skills() without extra_skills_dirs. After removing a custom command's only base while Codex is active, a previously written Claude skill can retain the removed body when Claude is reactivated: registration has no base/cache, and this deletion is skipped. The same problem occurs when the sole composer was disabled during removal; re-enabling it lets registration render its stale cache before reconciliation deletes only the cache. Move ownership-checked retirement outside the removal-only guard, and add regressions for both flows.

AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.

Handle native-skill aliases during recomposition and retirement

src/specify_cli/presets/_manager_skills.py:520

Native-skill aliases still retain removed instructions. CommandRegistrar.register_commands() renders a separate skill for each manifest alias (agents.py:934–1015), but this ownership scan and _register_skills() consider only the primary command's modern/legacy names. For an appending preset with alias speckit.shortcut, removing its base refreshes the primary skill but leaves speckit-shortcut/SKILL.md stale. Include aliases in recomposition and retirement, and recognize their registrar-generated source markers (<preset-id>:<source-file>), which differ from the primary skill's preset:<id> marker. Add a native-skill alias regression.

AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.

1 resolved since last review

Reject parent traversal in composed cache cleanup keys

_{Review effort: Max}

Copilot

🟡 Changes recommended

Temporarily setting the process-wide umask to 000 can race with overlapping registrations, leaving the mask at 000 and exposing outputs or later unrelated files through overly permissive access.

3 open issues

1 resolved since last review

Preserve restrictive permissions when writing command artifacts

_{Review effort: Balanced (auto)}

yxshee · 2026-11-06T16:31:03Z

Duplicate of the comment above; the fix for discussion_r4484399289 is f956449.

Copilot

🟡 Changes recommended

Unresolved composition and rendering bugs can lose or incorrectly rewrite skill content, while atomic-write regressions break long filenames and weaken file-permission protections.

4 open issues

2 resolved since last review

_{Review effort: Balanced (auto)}

+                            create=True,
+                            context="preset composed cache directory",
+                        )
+                        _write_shared_text(self.project_root, composed_file, composed)


mnriem · 2026-11-06T17:56:37Z

Please address Copilot feedback. If not applicable, please explain why. And also address test & lint errors

… umask Address four review findings: - _replace_file_text no longer reads the umask by setting it to 0. It creates the temp file with mode 0666 so the kernel applies the umask, and restores an existing file's own bits. - The temp name is a fixed short .speckit-<hex>.tmp, so a destination name near the filesystem limit stays writable. - The .composed cache refresh goes through the path guards, journaling and _replace_file_text, so an existing cache keeps its mode. - An alias belongs to the highest layer that declares it, not to the primary command's winning layer, so a lower declarer recomposes it on rescaffold and restores it when a new base arrives. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)

Copilot

🟡 Changes recommended

Replacing a private artifact can expose its contents to other local users because the temporary file is written with overly permissive permissions before the original mode is restored.

2 open issues

3 resolved since last review

_{Review effort: Balanced (auto)}

yxshee · 2026-11-08T14:26:28Z

Addressed the four findings from the latest Copilot reviews in 586db13.

Process umask changed to read it (discussion_r4494830290): _replace_file_text no longer calls os.umask. It creates the temp file with os.open(..., O_CREAT | O_EXCL, 0o666), so the kernel applies the current umask to a new output, and it chmods to the destination's own bits only when replacing an existing regular file.

Long filenames fail with ENAMETOOLONG (discussion_r4495630946): the temp entry is now .speckit-<16 hex>.tmp, a fixed 29 bytes, instead of the destination name plus a suffix. Any name that is valid for the destination stays writable.

Composed cache reset to 0644 (discussion_r4495631107): the .composed cache write in _reconcile_preset_aliases now goes through the same path guards, lifecycle journaling and _replace_file_text, so an existing cache keeps its mode. The SKILL.md writes in _register_skills that the comment also names use shared_infra._write_shared_text, which writes with a fixed 0644 on main too. That predates this PR and applies to every shared-infra write, so I left it out of scope here and can raise it as a separate issue if you want.

Alias precedence taken from the primary's winner (discussion_r4495631046): an alias now belongs to the highest layer that declares it (_layer_declares_alias), not to whichever layer wins the primary command. When a higher preset doesn't declare the alias, the lower declarer renders it from its own layer down, both on a rescaffold and when a new base makes it composable again. A higher declarer still shadows it as before.

New tests, each failing on f956449 and passing with the fix:

test_replace_keeps_long_valid_filename_writable, test_replace_creates_long_valid_filename (250-byte names)
test_replace_does_not_touch_process_umask
test_historical_alias_refresh_preserves_cache_mode
test_rescaffold_recomposes_alias_declared_only_by_lower_preset
test_new_base_restores_alias_declared_only_by_lower_preset

Tests and lint: the only red check on f956449 was pytest (macos-latest, 3.13). It failed in checkout (Could not resolve host: github.com), and the other pytest jobs were cancelled by fail-fast, so no test or lint failure came from the change. ruff was green there.

Verification on 586db13:

uv run pytest tests/specify_cli tests/test_presets.py tests/test_extensions.py tests/integrations tests/integration tests/extensions: 8847 passed, 147 skipped
uvx ruff@0.15.0 check src tests: clean
Merges cleanly with current main

AI disclosure: Claude Code worked autonomously on this round. It wrote the fixes, the tests and this summary, and ran the checks above. yxshee reviews the PR.

Replacing a 0600 file under a 022 umask created a 0644 temp file and wrote the new content before the final chmod restored the private mode. Create the temp file with the destination's own bits and set them before writing, so the content is never readable more widely than the destination. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)

yxshee · 2026-11-08T16:16:38Z

Addressed the latest Copilot finding in 53a373e.

Temp file briefly wider than a private destination (discussion_r4502724300): _replace_file_text now creates the temp file with the destination's own permission bits, not 0666, and sets them with chmod before any content is written. Under a 022 umask, replacing a 0600 file no longer writes the new text into a 0644 temp file first. New outputs still get 0666 under the process umask.

New test: test_replace_writes_private_content_only_into_private_temp records the temp file's mode at the moment it is opened for writing. On 586db13 it is 0644, and with the fix it is 0600.

Verification on 53a373e:

uv run pytest tests/specify_cli tests/test_presets.py tests/test_extensions.py tests/integrations tests/integration tests/extensions: 8848 passed, 147 skipped
uvx ruff@0.15.0 check src tests: clean
Merges cleanly with current main

AI disclosure: Claude Code (Claude Opus 5.5) worked autonomously on this round. It wrote the fix, the test and this summary, and ran the checks above. yxshee reviews the PR.

Copilot

🟡 Changes recommended

If os.chmod() fails, the temporary file descriptor remains open, allowing leaks to accumulate when registration catches errors and continues.

2 open issues

1 resolved since last review

Preserve restrictive permissions on replacement temporary files

_{Review effort: Balanced (auto)}

_replace_file_text restored the destination's permission bits on the temp file before wrapping the descriptor, so a failing os.chmod left the descriptor open; only the temp entry was removed. Registration catches write errors and continues, so the leak accumulated. Enter the file context first and restore the mode inside it, still before any content is written. Assisted-by: Claude Code (model: Claude Mythos 5.1, autonomous)

yxshee · 2026-11-08T20:10:51Z

Addressed the latest Copilot finding in a6a344f.

Descriptor leaked when chmod fails (discussion_r4506905678): _replace_file_text now enters the os.fdopen context first and restores the destination's permission bits inside it, before any content is written. If os.chmod raises, the context closes the descriptor and the cleanup still removes the temp file, so a registration pass that catches the error and continues no longer accumulates open descriptors. The ordering guarantee from 53a373e is unchanged: the mode is still set before the first write.

New test: test_replace_closes_descriptor_when_chmod_fails makes os.chmod raise, then checks the temp descriptor is closed, the destination is untouched and no temp file is left behind. On 53a373e the descriptor is still open after the error; with the fix it is closed.

Verification on a6a344f:

uv run pytest tests/specify_cli tests/test_presets.py tests/test_extensions.py tests/integrations tests/integration tests/extensions: 8852 passed, 143 skipped, and test_resolve_core_does_not_return_preset_files failing only because this worktree lives in a directory named presets (it asserts no presets segment in the bundled template's absolute path); it passes from a checkout outside such a path, as do the five _replace_file_text tests
uvx ruff@0.15.0 check src tests: clean
Merges cleanly with current main

AI disclosure: Claude Code (Claude Mythos 5.1) worked autonomously on this round. It wrote the fix, the test and this summary, and ran the checks above. yxshee reviews the PR.

Copilot

🔵 Needs a closer look

Human assessment is needed to evaluate permission and ACL preservation risks that could expose previously restricted instructions to other local users during refreshes.

1 open issue

Preserve file permissions during atomic writes

1 resolved since last review

Close the file descriptor if chmod fails

_{Review effort: Balanced (auto)}

mnriem · 2026-11-09T14:00:03Z

Please address Copilot feedback

The skill writes in the preset skills manager went through _write_shared_text, which always set 0644, so recomposing or restoring a private (0600) SKILL.md on preset removal made it world-readable. Add an opt-in preserve_mode to _write_shared_text that keeps an existing regular file's bits (new files still get 0644) and use it for every SKILL.md write there. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)

yxshee · 2026-11-09T16:17:26Z

Addressed the one issue the latest Copilot review on a6a344f still lists as open, "Preserve file permissions during atomic writes" (discussion_r4495631107), in daac335.

Earlier I left the SKILL.md part of that thread out of scope. On a second look, this PR's removal-time refreshes do rewrite those files, so I fixed it here. _write_shared_text gets an opt-in preserve_mode: an existing regular file keeps its permission bits instead of being reset to 0644, and a new file still gets 0644 as before. The content is still written into a 0600 temp file and renamed into place, so it is never readable more widely than the destination. Every SKILL.md write in the preset skills manager (register, override restore, core and extension restore) now passes it. Other shared-infra writers are unchanged.

New tests, both failing on a6a344f (the file ended up 0644) and passing with the fix:

test_skill_refresh_on_removal_preserves_skill_mode: a 0600 skill recomposed onto core after the base is removed stays 0600
test_skill_restore_on_removal_preserves_skill_mode: a 0600 skill restored from core when the last preset goes stays 0600

On ACLs, which the review summary also mentions: an atomic rename gives the file a new inode, so POSIX ACLs and extended attributes on the old file don't carry over. That holds for every atomic write in the project, including _write_shared_bytes on main, and fixing it needs per-platform ACL copying. I've left it out of scope for this PR and can open an issue if you want it tracked.

Verification on daac335:

uv run pytest tests/specify_cli tests/test_presets.py tests/test_extensions.py tests/integrations tests/integration tests/extensions: 8851 passed, 147 skipped
uvx ruff@0.15.0 check src tests: clean
Merges cleanly with current main

AI disclosure: Claude Code (Claude Opus 5.5) worked autonomously on this round. It wrote the fix, the tests and this summary, and ran the checks above. yxshee reviews the PR.

Copilot

🔵 Needs a closer look

Human assessment is needed to evaluate the critical temporary-file substitution risk and multiple state-dependent skill reconciliation failures identified in the omitted findings.

1 open issue

Preserve file permissions during atomic writes

_{Review effort: Balanced (auto)}

mnriem · 2026-11-09T18:52:54Z

Please address Copilot feedback and resolve conflicts

main now keeps user files next to a generated SKILL.md when a preset skill is removed (github#5233) and dropped the shutil import. Use the same unlink_in_directory() helper for the stale composed skills and aliases this branch retires, instead of removing the whole skill directory.

yxshee · 2026-11-09T20:15:11Z

Merged main in 390a6c4 and resolved the import conflict in test_manager_skills.py. #5233 changed preset skill removal to delete only SKILL.md and dropped the shutil import, so 1b81d0a uses the same unlink_in_directory() for the skills and aliases this PR retires. The one open Copilot thread is the permissions one fixed in daac335, and the Windows failure in test_script_help_includes_python_variant also happens on main (run 42678554323).

Copilot

🟡 Changes recommended

Unresolved artifact-replacement flaws can discard ownership and ACL protections or permit temporary-entry substitution, risking restricted-content exposure and installation of substituted artifacts.

5 open issues

_{Review effort: Balanced (auto)}

+                    os.chmod(temp_path, keep_mode)
+                fh.write(content)
+            os.replace(temp_path, dest)


+                    os.chmod(temp_path, keep_mode)
+                fh.write(content)
+            os.replace(temp_path, dest)


+                owner_layers = layers[owner_idx:]
+                composed = resolver.resolve_content(
+                    cmd_name, "command", layers=owner_layers
+                )


+                    skill_name = registrar._compute_output_name(agent, alias, agent_config)
+                    skill_dir = skills_dir / skill_name
+                    skill_file = skill_dir / "SKILL.md"
+                    missing = not skill_dir.exists() and not skill_dir.is_symlink()


mnriem · 2026-11-09T20:52:15Z

Please address Copilot feedback

yxshee requested a review from mnriem as a code owner October 17, 2026 19:15

mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Oct 18, 2026

mnriem requested a max review from Copilot October 19, 2026 13:03

Copilot started reviewing on behalf of mnriem October 19, 2026 13:04

View session

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/specify_cli/presets/_manager_skills.py Resolved

mnriem requested a max review from Copilot October 19, 2026 13:59

Copilot started reviewing on behalf of mnriem October 19, 2026 14:00

View session

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/specify_cli/presets/_manager_skills.py Resolved

mnriem added the author-awaiting Waiting on author response label Oct 19, 2026

mnriem requested a max review from Copilot October 19, 2026 14:30

Copilot started reviewing on behalf of mnriem October 19, 2026 14:30

View session

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/specify_cli/presets/_manager_skills.py Resolved

Comment thread src/specify_cli/presets/_manager_skills.py Resolved

Comment thread src/specify_cli/presets/_manager_skills.py Resolved

mnriem requested a max review from Copilot October 19, 2026 16:56

Copilot started reviewing on behalf of mnriem October 19, 2026 16:57

View session

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/specify_cli/presets/_manager_skills.py Resolved

mnriem requested a max review from Copilot October 19, 2026 17:41

Copilot started reviewing on behalf of mnriem October 19, 2026 17:41

View session

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/specify_cli/presets/_manager_skills.py Resolved

Copilot AI reviewed Nov 6, 2026

View reviewed changes

Comment thread src/specify_cli/agents.py Resolved

mnriem requested an auto review from Copilot November 6, 2026 17:30

Copilot started reviewing on behalf of mnriem November 6, 2026 17:30

View session

Copilot AI reviewed Nov 6, 2026

View reviewed changes

Copilot AI auto review requested due to automatic review settings November 7, 2026 16:18

Copilot started reviewing on behalf of yxshee November 7, 2026 16:19

View session

Copilot AI reviewed Nov 7, 2026

View reviewed changes

Comment thread src/specify_cli/agents.py Resolved

Copilot AI auto review requested due to automatic review settings November 8, 2026 16:16

Copilot started reviewing on behalf of yxshee November 8, 2026 16:16

View session

Copilot AI reviewed Nov 8, 2026

View reviewed changes

Comment thread src/specify_cli/agents.py Resolved

Copilot AI auto review requested due to automatic review settings November 8, 2026 20:10

Copilot AI reviewed Nov 8, 2026

View reviewed changes

Copilot AI reviewed Nov 9, 2026

View reviewed changes

yxshee added 2 commits November 10, 2026 01:44

Merge branch 'main' into fix/preset-remove-stale-composed-skill

390a6c4

Copilot AI reviewed Nov 9, 2026

View reviewed changes

Conversation

yxshee commented Oct 17, 2026

Description

Testing

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

yxshee commented Oct 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

mnriem commented Oct 19, 2026

Uh oh!

yxshee commented Oct 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnriem commented Oct 19, 2026

Uh oh!

yxshee commented Oct 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

mnriem commented Oct 19, 2026

Uh oh!

yxshee commented Oct 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

yxshee commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

mnriem commented Nov 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

yxshee commented Nov 8, 2026

Uh oh!

yxshee commented Nov 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

yxshee commented Nov 8, 2026

Uh oh!

Copilot AI left a comment

yxshee commented Nov 6, 2026 •

edited

Loading